Conversation
WalkthroughThis PR refactors blocked-seller filtering logic from the database query layer to the service/application layer. It extracts the logged-in user ID in the controller, passes it through service methods, and uses it to fetch blocked seller IDs post-query, marking items with a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/com/salemale/domain/item/service/ItemService.java (1)
450-453: Recommended items bypass theblockedSellerflag transformation.
RecommendationService.getRecommendedItems()usesItemConverter.toAuctionListItemDTO(item)(line 129), which calls the single-parameter overload that does not set theblockedSellerflag. This differs from the standard auction list flow (lines 390-398) which computes blocked seller IDs and explicitly passes them to the two-parameter overloadItemConverter.toAuctionListItemDTO(item, blockedSeller).As a result, recommended items will always have
blockedSeller=falseregardless of whether the seller is actually blocked, creating inconsistent behavior between standard and recommended item listings.Pass
blockedSellerIdstoRecommendationService.getRecommendedItems()and apply the flag during DTO conversion, matching the pattern used in the standard auction list flow.src/main/java/com/salemale/domain/search/service/KeywordItemSearchServiceImpl.java (1)
236-236: Unused userId parameter in helper methods.Both
searchCompletedItemsandsearchCompletedItemsNearbyaccept auserIdparameter (lines 236, 271) but never use it. These parameters appear to be remnants from the previous repository-level filtering approach. Consider removing them from the method signatures since blocked-seller handling is now performed in the calling code (lines 128-132).Apply this diff to remove the unused parameter from
searchCompletedItems:-private Page<Item> searchCompletedItems(String keyword, java.util.List<Category> categories, Integer minPrice, Integer maxPrice, org.springframework.data.domain.Pageable pageable, Long userId) { +private Page<Item> searchCompletedItems(String keyword, java.util.List<Category> categories, Integer minPrice, Integer maxPrice, org.springframework.data.domain.Pageable pageable) {And update the calls on lines 124 and 125:
- ? searchCompletedItems(keyword, cats, normalizeMin(minPrice), normalizeMax(maxPrice), sorted, userId) - : searchCompletedItemsNearby(keyword, lat, lon, km, cats, normalizeMin(minPrice), normalizeMax(maxPrice), sort, pageable, userId); + ? searchCompletedItems(keyword, cats, normalizeMin(minPrice), normalizeMax(maxPrice), sorted) + : searchCompletedItemsNearby(keyword, lat, lon, km, cats, normalizeMin(minPrice), normalizeMax(maxPrice), sort, pageable);Apply similar changes for
searchCompletedItemsNearby:-private Page<Item> searchCompletedItemsNearby(String keyword, double lat, double lon, double km, java.util.List<Category> categories, Integer minPrice, Integer maxPrice, AuctionSortType sort, Pageable pageable, Long userId) { +private Page<Item> searchCompletedItemsNearby(String keyword, double lat, double lon, double km, java.util.List<Category> categories, Integer minPrice, Integer maxPrice, AuctionSortType sort, Pageable pageable) {Also applies to: 271-271
🧹 Nitpick comments (4)
src/main/java/com/salemale/domain/search/service/NearbyItemSearchServiceImpl.java (1)
45-55: Consider usingSet<Long>instead ofList<Long>for blocked seller IDs.Using
List.contains()inside the mapping loop is O(n) per item, which can degrade performance when the block list is large.ItemServicealready usesSet<Long>for this purpose (see Lines 381-388 in ItemService.java). Using aSethere would provide O(1) lookup and maintain consistency across services.// 내가 차단한 판매자 ID 목록 - List<Long> blockedSellerIds = - blockListRepository.findBlockedUserIds(userId); + Set<Long> blockedSellerIds = new HashSet<>( + blockListRepository.findBlockedUserIds(userId)); Page<Item> page = itemRepository.findNearbyItems(ItemStatus.BIDDING.name(), lat, lon, km, pageable); return page.map(item -> { boolean blockedSeller = blockedSellerIds.contains(item.getSeller().getId()); return ItemConverter.toAuctionListItemDTO(item, blockedSeller); });Add import:
import java.util.HashSet; import java.util.Set;src/main/java/com/salemale/domain/item/converter/ItemConverter.java (1)
171-213: Reduce code duplication by delegating to the new overload.The two
toAuctionListItemDTOmethods share identical logic except for theblockedSellerfield. To improve maintainability and follow DRY principles, consider having the original method delegate to the new one.public static AuctionListItemDTO toAuctionListItemDTO(Item item) { - // 전체 이미지 사용 - List<String> imageUrls = item.getImages().stream() - .map(ItemImage::getImageUrl) - .collect(Collectors.toList()); - - return AuctionListItemDTO.builder() - .itemId(item.getItemId()) - .title(item.getTitle()) - .imageUrls(imageUrls) - .currentPrice(item.getCurrentPrice()) - .bidderCount(item.getBidCount()) - .endTime(item.getEndTime()) - .viewCount(item.getViewCount()) - .itemStatus(item.getItemStatus().name()) - .startPrice(item.getStartPrice()) // 시작가 - .createdAt(item.getCreatedAt()) // 시작날짜 - .build(); + return toAuctionListItemDTO(item, false); }src/main/java/com/salemale/domain/item/controller/ItemController.java (1)
188-205: Potential redundant call and overly broad exception handling.Two observations:
Redundant
getCurrentUserIdcall:loginUserIdis already extracted (lines 190-195), but forRECOMMENDEDstatus,getCurrentUserIdis called again (line 199). Consider reusingloginUserIdwith a null check.Broad exception suppression: Catching all
Exceptiontypes could hide unexpected errors. Consider catching only the specific authentication exception type.// 로그인 안 했으면 null // 로그인 했으면 userId Long loginUserId = null; try { loginUserId = currentUserProvider.getCurrentUserId(request); - } catch (Exception e) { + } catch (GeneralException e) { // 비로그인 요청이면 그대로 null 유지 } // 추가: RECOMMENDED 상태일 때는 별도 처리 if (status == AuctionStatus.RECOMMENDED) { - Long userId = currentUserProvider.getCurrentUserId(request); + if (loginUserId == null) { + throw new GeneralException(ErrorStatus.USER_NOT_FOUND); + } - AuctionListResponse response = itemService.getRecommendedAuctionList(userId, pageable); + AuctionListResponse response = itemService.getRecommendedAuctionList(loginUserId, pageable); return ResponseEntity.ok(ApiResponse.onSuccess(response)); }src/main/java/com/salemale/domain/search/service/KeywordItemSearchServiceImpl.java (1)
174-202: Review in-memory filtering impact on pagination.The nearby search applies category, price, and POPULAR filters in memory after DB pagination (lines 175-183). This can result in fewer items being returned than the requested page size, leading to inconsistent pagination behavior. The comment at line 191 states "페이지네이션 유지" (maintain pagination) and claims only sorting is adjusted, but the code clearly performs filtering as well.
While this appears to be pre-existing logic (only the DTO mapping changed in this PR), consider refactoring to apply these filters at the repository level to maintain accurate pagination, or adjust the comment to reflect the actual behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/java/com/salemale/domain/item/controller/ItemController.java(2 hunks)src/main/java/com/salemale/domain/item/converter/ItemConverter.java(1 hunks)src/main/java/com/salemale/domain/item/dto/response/AuctionListItemDTO.java(1 hunks)src/main/java/com/salemale/domain/item/repository/ItemRepository.java(0 hunks)src/main/java/com/salemale/domain/item/service/ItemService.java(5 hunks)src/main/java/com/salemale/domain/search/service/KeywordItemSearchServiceImpl.java(12 hunks)src/main/java/com/salemale/domain/search/service/NearbyItemSearchServiceImpl.java(3 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/com/salemale/domain/item/repository/ItemRepository.java
🧰 Additional context used
🧬 Code graph analysis (4)
src/main/java/com/salemale/domain/item/dto/response/AuctionListItemDTO.java (1)
src/main/java/com/salemale/domain/item/dto/response/AuctionListResponse.java (1)
Schema(14-41)
src/main/java/com/salemale/domain/search/service/NearbyItemSearchServiceImpl.java (2)
src/main/java/com/salemale/domain/search/service/KeywordItemSearchServiceImpl.java (1)
Service(27-352)src/main/java/com/salemale/domain/item/converter/ItemConverter.java (1)
ItemConverter(21-282)
src/main/java/com/salemale/domain/search/service/KeywordItemSearchServiceImpl.java (1)
src/main/java/com/salemale/domain/item/converter/ItemConverter.java (1)
ItemConverter(21-282)
src/main/java/com/salemale/domain/item/service/ItemService.java (1)
src/main/java/com/salemale/domain/item/converter/ItemConverter.java (1)
ItemConverter(21-282)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (9)
src/main/java/com/salemale/domain/item/dto/response/AuctionListItemDTO.java (1)
51-54: LGTM!The new
blockedSellerfield is well-implemented. Using a primitivebooleanensures it defaults tofalsefor cases where the user is not logged in, which aligns with the PR's design intent.src/main/java/com/salemale/domain/item/service/ItemService.java (1)
380-398: LGTM!Good implementation using
Set<Long>for O(1) lookup performance. The pattern of using a temp variable then assigning tofinalfor lambda capture is correct.src/main/java/com/salemale/domain/search/service/KeywordItemSearchServiceImpl.java (7)
13-13: LGTM!The
BlockListRepositorydependency is correctly injected following the existing constructor injection pattern.Also applies to: 34-34
99-101: LGTM!Blocked seller IDs are efficiently retrieved once per search request after user validation. The placement ensures the list is available for all subsequent DTO mappings.
64-88: LGTM! Non-logged-in users correctly use the simple DTO converter.For non-logged-in users, the simple
ItemConverter::toAuctionListItemDTOmethod is used, which doesn't include theblockedSellerflag. This is appropriate since non-logged-in users cannot have block lists.
128-132: LGTM!Completed items for logged-in users are correctly mapped with the
blockedSellerflag by checking if the seller's ID is in the blocked list.
193-199: LGTM!Nearby search results for logged-in users are correctly mapped with the
blockedSellerflag.
204-208: LGTM!Nationwide search results for logged-in users are correctly mapped with the
blockedSellerflag.
338-351:and
🎋 이슈 및 작업중인 브랜치
#146
🔑 주요 내용
-차단한 사용자가 등록한 아이템을 보이지 않게 하던 방법에서 아이템에 차단한 사용자가 올린 아이템인지 알아볼 수 있게 플래그 생성


-처음에 itemrepository 쿼리를 수정하여 아이템을 필터링했던 방법의 문제점 : native query와 jpql의 충돌이 발생했는데 전체 수정하지 않는 이상 해결 불가
-그래서 두번째로 service 차원에서 필터링하려 하였으나 range=ALL일 때는 service 단계에서 차단할 수가 없음, page를 filter로 줄이면서 데이터 불일치 -> 스웨거 상에서 items =[] 비어있는데 totalElements=2와 같은 응답 발생
-최종적으로 차단한 판매자 아이템도 그대로 보이게 하되, 시각적으로 알아볼 수 있게 차단한 사용자가 등록했는지 플래그를 생성
순서대로 차단하지 않았을 때, 차단 했을 때의 캡쳐
"blockedSeller" = false/true
Check List
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.